Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols #143669

jieyouxu · 2025-07-09T05:39:11Z

This PR fixes the tests/run-make/fmt-write-float/ run-make test to both (1) not vacuously pass on no symbols at all, and (2) actually check aginst the platform-specific decorated panic symbols.

Commit 1 introduces a new dependency on getsentry/pdb in the run-make-support library to allow us to programmatically inspect PDB files, instead of trying to perform textual output matching on llvm-pdbutil whose textual output is not necessarily guaranteed to remain the same between versions.
Commit 2 both adds a smoke check to make sure that the executable (or PDB file) we're inspecting actually has a main symbol and isn't completely devoid of symbols, and also fixes the negative check against panic machinery symbols to account for platform differences.

Context

Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion.

Fix this by checking we at least observe the main symbol which is always expected to be present.

Noticed in #142841 (comment).

r? @ChrisDenton

try-job: aarch64-apple
try-job: x86_64-apple-1
try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: x86_64-mingw-1

ChrisDenton · 2025-07-09T07:41:35Z

@bors2 try jobs=x86_64-msvc-*

rust-bors · 2025-07-09T07:41:41Z

⌛ Trying commit 5cef54f with merge 9306675…

To cancel the try build, run the command @bors2 try cancel.

Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: x86_64-msvc-*

rust-bors · 2025-07-09T10:18:49Z

💔 Test failed

CI ❌

jieyouxu · 2025-07-09T10:21:48Z

Hm. Let me try this locally on windows.

ChrisDenton · 2025-07-09T10:22:51Z

The failure is not surprising to me. On Windows, symbols are not contained in the binary itself and are instead in a separate file (the .pdb). So the object crate would not be able to find them.

jieyouxu · 2025-07-09T10:40:01Z

Oh of course... D'oh. Let me see if I can cook something up.

rustbot · 2025-07-09T11:41:29Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

jieyouxu · 2025-07-09T11:43:26Z

@bors2 try

rust-bors · 2025-07-09T11:43:31Z

⌛ Trying commit 63b9636 with merge a1b33a4…

To cancel the try build, run the command @bors2 try cancel.

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

jieyouxu · 2025-07-09T11:44:09Z

llvm-pdbutil is an fallback approach, trying pdb crate to see if that works under CI conditions.

jieyouxu · 2025-07-09T11:49:48Z

I bet this will also fail on apple because _ prefix to symbol names.

jieyouxu · 2025-07-09T12:22:30Z

I forgor to run this locally, it's is_windows_msvc not is_msvc.
@bors2 try cancel

rust-bors · 2025-07-09T12:22:34Z

Try build cancelled. Cancelled workflows:

https://github.com/rust-lang/rust/actions/runs/16168390696

jieyouxu · 2025-07-09T12:25:42Z

Oh. Apparently we have both is_msvc and is_windows_msvc. I'll fix that separately.

@bors2 try

rust-bors · 2025-07-09T12:25:47Z

⌛ Trying commit 63b9636 with merge 1d2e78e…

To cancel the try build, run the command @bors2 try cancel.

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present.

jieyouxu · 2025-07-10T10:54:54Z

@bors2 try

rust-bors · 2025-07-10T10:54:59Z

⌛ Trying commit 24830de with merge f734e9c…

To cancel the try build, run the command @bors2 try cancel.

[WIP] Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols Previously, the test only checks for the absence of certain panic symbols, but having no symbols was also a possible albeit vacuous way to satisfy this assertion. Fix this by checking we at least observe the `main` symbol which is always expected to be present. Noticed in #142841 (comment). r? `@ChrisDenton` try-job: aarch64-apple try-job: x86_64-apple-1 try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1

Assorted `run-make-support` maintenance This PR should contain no functional changes. - Commit 1: Removes the support library's CHANGELOG. In the very beginning, I thought maybe we would try to version this library. But this is a purely internal test support library, and it's just extra busywork trying to maintain changelog/versions. It's also hopelessly outdated. - Commit 2: Resets version number to `0.0.0`. Ditto on busywork. - Commit 3: Bump `run-make-support` to Edition 2024. The support library was already "compliant" with Edition 2024. - Commit 4: Slightly organizes the support library dependencies. - Commit 5: Previously, I tried hopelessly to maintain some manual formatting, but that was annoying because it required skipping rustfmt (so export ordering etc. could not be extra formatted). Give up, and do some rearrangements / module prefix tricks to get the `lib.rs` looking at least *reasonable*. IMO this is not a strict improvement, but I rather regain the ability to auto-format it with rustfmt. - Commit {6,7}: Noticed in rust-lang#143669 that we apparently had *both* {`is_msvc`, `is_windows_msvc`}. This PR removes `is_msvc` in favor of `is_windows_msvc` to make it unambiguous (and only retain one way of gating) as there are some UEFI targets which are MSVC but not Windows. Best reviewed commit-by-commit. r? `@Kobzol`

rust-bors · 2025-07-10T13:56:14Z

☀️ Try build successful (CI)
Build commit: f734e9c (f734e9c362b13118d196e6a5f983e9c8cf7f4a32, parent: cf3fb768db439825e3c8d327f6d9f46e02965668)

jieyouxu · 2025-07-10T15:38:15Z

Try jobs are green.
@rustbot review

jieyouxu · 2025-07-10T15:39:15Z

Re. the pdb crate dependency, other run-make tests use llvm-pdbutil currently, but matching against its textual output is IMO quite fragile. It's probably possible to migrate them over to also use pdb programmatically, but beyond the scope of this PR.

ChrisDenton · 2025-07-10T15:43:50Z

My only concern with the pdb crate is it doesn't look like it's been updated in awhile. It's probably fine (it has few dependencies by the looks of it) but it makes me slightly wary.

I'm not going to insist on it on this PR but I think long term it would be good to use official debugging APIs on Windows hosts and only fallback to the pdb crate on other platforms (or maybe if they're unavailable for some reason).

ChrisDenton

Thinking about this some more, maybe a more robust way to do this test is to compile two programs: one with the symbols and one without. That would guard against, e.g. changing symbol names or different kinds of naming conventions.

tests/run-make/fmt-write-bloat/rmake.rs

ChrisDenton · 2025-07-10T15:50:40Z

tests/run-make/fmt-write-bloat/rmake.rs

+        assert!(any_symbol_contains(bin_name("main"), &["main"]));
+        assert!(!any_symbol_contains(bin_name("main"), &panic_syms));


wait, I know this is pre-existing but why is this bin_name? That should be a no-op on unix targets but main.exe on Windows, which sounds weird.

I think it's just because the underlying helper uses fs::read, which doesn't try to account for .exe or not.

jieyouxu · 2025-07-10T15:55:26Z

My only concern with the pdb crate is it doesn't look like it's been updated in awhile. It's probably fine (it has few dependencies by the looks of it) but it makes me slightly wary.

I'm not going to insist on it on this PR but I think long term it would be good to use official debugging APIs on Windows hosts and only fallback to the pdb crate on other platforms (or maybe if they're unavailable for some reason).

Ah, did you mean the Debug Interface Access SDK?

ChrisDenton · 2025-07-10T15:57:00Z

Yeah. But I mean, figuring out how to use it is perhaps more work then you signed up for 😉.

jieyouxu · 2025-07-10T15:59:00Z

Hm yeah. I think in the long-term, we should probably be using DIA wrappers (but that's quite a bit of Windows-specific work). In the short-term, I could also fallback to just using llvm-pdbutil for now, since if the long-term plan is to use DIA, then it seems better to not introduce a dependency on pdb.

EDIT: #143737

jieyouxu · 2025-07-10T16:00:19Z

Let me try the

compile two programs: one with the symbols and one without. That would guard against, e.g. changing symbol names or different kinds of naming conventions.

approach.

@rustbot author

Rollup merge of #143683 - jieyouxu:rms-cleanup, r=Kobzol Assorted `run-make-support` maintenance This PR should contain no functional changes. - Commit 1: Removes the support library's CHANGELOG. In the very beginning, I thought maybe we would try to version this library. But this is a purely internal test support library, and it's just extra busywork trying to maintain changelog/versions. It's also hopelessly outdated. - Commit 2: Resets version number to `0.0.0`. Ditto on busywork. - Commit 3: Bump `run-make-support` to Edition 2024. The support library was already "compliant" with Edition 2024. - Commit 4: Slightly organizes the support library dependencies. - Commit 5: Previously, I tried hopelessly to maintain some manual formatting, but that was annoying because it required skipping rustfmt (so export ordering etc. could not be extra formatted). Give up, and do some rearrangements / module prefix tricks to get the `lib.rs` looking at least *reasonable*. IMO this is not a strict improvement, but I rather regain the ability to auto-format it with rustfmt. - Commit {6,7}: Noticed in #143669 that we apparently had *both* {`is_msvc`, `is_windows_msvc`}. This PR removes `is_msvc` in favor of `is_windows_msvc` to make it unambiguous (and only retain one way of gating) as there are some UEFI targets which are MSVC but not Windows. Best reviewed commit-by-commit. r? `@Kobzol`

bors · 2025-07-10T17:54:16Z

☔ The latest upstream changes (presumably #143731) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2025-07-11T11:52:47Z

tests/run-make/fmt-write-bloat/rmake.rs


 fn main() {
    rustc().input("main.rs").opt().run();
    // panic machinery identifiers, these should not appear in the final binary
-    let mut panic_syms = vec!["panic_bounds_check", "Debug"];
+    let mut panic_syms = vec![sym("panic_bounds_check"), sym("Debug")];
    if no_debug_assertions() {


Remark: while investigating this test, I realized this is actually incorrect (even in the original Makefile version), and the port preserved the incorrect gating

# Allow for debug_assert!() in debug builds of std. ifdef NO_DEBUG_ASSERTIONS PANIC_SYMS += panicking panic_fmt pad_integral Display Debug endif

I believe NO_DEBUG_ASSERTIONS corresponds (confusingly) only to whether rustc has debug assertions enabled, not std.

I have a separate PR that I'll send first.

jieyouxu · 2025-07-12T13:24:44Z

Adjusting the symbols helpers first in #143837.

…Denton Disambiguate between rustc vs std having debug assertions in `run-make-support` and `run-make` tests `NO_DEBUG_ASSERTIONS` is set by CI that threads through to the `./configure.py` script, which is somewhat fragile and "spooky action at a distance". For `fmt-write-bloat`, this is actually wrong because the test wants to gate on *std* being built with debug assertions or not, whereas `NO_DEBUG_ASSERTIONS` determines *rustc* being built with debug assertions or not. Instead, use env vars controlled by compiletest, whose debug assertion info comes from bootstrap. https://github.com/rust-lang/rust/blob/855e0fe46e68d94e9f6147531b75ac2d488c548e/src/ci/run.sh#L137-L146 `NO_DEBUG_ASSERTIONS` controls `--enable-debug-assertions` https://github.com/rust-lang/rust/blob/855e0fe46e68d94e9f6147531b75ac2d488c548e/src/bootstrap/configure.py#L124 which sets `--rust.debug-assertions`, which controls *rustc* debug assertions. https://github.com/rust-lang/rust/blob/855e0fe46e68d94e9f6147531b75ac2d488c548e/src/bootstrap/configure.py#L125-L129 `--rust.debug-assertions-std` controls *std* debug assertions. Noticed while investigating `fmt-write-bloat` in rust-lang#143669 (comment). Best reviewed commit-by-commit. r? `@ChrisDenton` (or compiler/bootstrap)

rustbot assigned ChrisDenton Jul 9, 2025

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 9, 2025

jieyouxu force-pushed the fmt-write-bloat branch from c6da322 to 5cef54f Compare July 9, 2025 05:39

jieyouxu mentioned this pull request Jul 9, 2025

Enable fmt-write-bloat for Windows #142841

Merged

This comment has been minimized.

Sign in to view

jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2025

jieyouxu changed the title ~~Make sure fmt-write-bloat doesn't vacuously pass on no symbols~~ [WIP] Make sure fmt-write-bloat doesn't vacuously pass on no symbols Jul 9, 2025

jieyouxu mentioned this pull request Jul 9, 2025

Assorted run-make-support maintenance #143683

Merged

jieyouxu force-pushed the fmt-write-bloat branch from b63f2b1 to 24830de Compare July 10, 2025 10:54

jieyouxu changed the title ~~[WIP] Make sure fmt-write-bloat doesn't vacuously pass on no symbols~~ Make sure fmt-write-bloat doesn't vacuously pass on no symbols Jul 10, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2025

ChrisDenton reviewed Jul 10, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 10, 2025

jieyouxu mentioned this pull request Jul 10, 2025

Consider adding Windows Debug Interface Access (DIA) SDK wrappers for PDB analysis #143737

Open

jieyouxu commented Jul 11, 2025

View reviewed changes

This was referenced Jul 11, 2025

Disambiguate between rustc vs std having debug assertions in run-make-support and run-make tests #143782

Open

Adjust run-make-support::symbols helpers #143837

Open

jieyouxu added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2025

		assert!(any_symbol_contains(bin_name("main"), &["main"]));
		assert!(!any_symbol_contains(bin_name("main"), &panic_syms));

Make sure fmt-write-bloat doesn't vacuously pass on no symbols #143669

Are you sure you want to change the base?

Make sure fmt-write-bloat doesn't vacuously pass on no symbols #143669

Conversation

jieyouxu commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Uh oh!

This comment has been minimized.

This comment has been minimized.

ChrisDenton commented Jul 9, 2025

Uh oh!

rust-bors bot commented Jul 9, 2025

Uh oh!

rust-bors bot commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

ChrisDenton commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

rust-bors bot commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

rust-bors bot commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 9, 2025

Uh oh!

rust-bors bot commented Jul 9, 2025

Uh oh!

jieyouxu commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

rust-bors bot commented Jul 10, 2025

Uh oh!

jieyouxu commented Jul 10, 2025

Uh oh!

jieyouxu commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisDenton commented Jul 10, 2025

Uh oh!

ChrisDenton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisDenton Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Jul 10, 2025

Uh oh!

ChrisDenton commented Jul 10, 2025

Uh oh!

jieyouxu commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu commented Jul 10, 2025

Uh oh!

bors commented Jul 10, 2025

Uh oh!

jieyouxu Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Jul 11, 2025

Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols #143669

Make sure `fmt-write-bloat` doesn't vacuously pass on no symbols #143669

jieyouxu commented Jul 9, 2025 •

edited

Loading

jieyouxu commented Jul 9, 2025 •

edited

Loading

jieyouxu commented Jul 10, 2025 •

edited

Loading

jieyouxu commented Jul 10, 2025 •

edited

Loading

jieyouxu Jul 11, 2025 •

edited

Loading